Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

codegen: limited support for oneOfVariant in responses #3993

Merged
merged 1 commit into from
Sep 2, 2024
Merged

codegen: limited support for oneOfVariant in responses #3993

merged 1 commit into from
Sep 2, 2024

Conversation

hughsimpson
Copy link
Contributor

@hughsimpson hughsimpson commented Aug 20, 2024 •
edited
Loading

Adding a bit of support for oneOfVariants. Where the previous code would've generated

.errorOut(jsonBody[MessageResponse].description("Bad Request").and(statusCode(sttp.model.StatusCode(400))))
.errorOut(jsonBody[MessageResponse].description("Forbidden").and(statusCode(sttp.model.StatusCode(403))))
.errorOut(jsonBody[MessageResponse].description("Not Found").and(statusCode(sttp.model.StatusCode(404))))
.errorOut(jsonBody[MessageResponse].description("Unauthorized").and(statusCode(sttp.model.StatusCode(401))))

we would now generate

.errorOut(oneOf(oneOfVariant(sttp.model.StatusCode(400), jsonBody[MessageResponse].description("Bad Request")), oneOfVariant(sttp.model.StatusCode(404), jsonBody[MessageResponse].description("Not Found")), oneOfVariant(sttp.model.StatusCode(403), jsonBody[MessageResponse].description("Forbidden")), oneOfVariant(sttp.model.StatusCode(401), jsonBody[MessageResponse].description("Unauthorized"))))

given

      responses:
        '400':
          description: Bad Request
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/MessageResponse'
        '401':
          description: Unauthorized
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/MessageResponse'
        '403':
          description: Forbidden
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/MessageResponse'
        '404':
          description: Not Found
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/MessageResponse'

hughsimpson
hughsimpson commented Aug 20, 2024
View reviewed changes
openapi-codegen/core/src/main/scala/sttp/tapir/codegen/SchemaGenerator.scala
else throw new NotImplementedError("any not implemented for json libs other than circe")
else
throw new NotImplementedError(
s"any not implemented for json libs other than circe (problematic models: ${schemasWithAny.map(_._1)})"
Copy link
Contributor Author

@hughsimpson hughsimpson Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely unrelated, but this can be triggered by surprising things (e.g. an object containing a property that's an enum without a type) so this helps avoid banging heads on walls or keyboards.

@adamw adamw merged commit 8995b1c into softwaremill:master Sep 2, 2024
22 checks passed
@adamw
Copy link
Member

adamw commented Sep 2, 2024

Thank you :)

@hughsimpson hughsimpson deleted the codegen_oneOfVariant branch September 2, 2024 15:14
hughsimpson
hughsimpson commented Nov 28, 2024
View reviewed changes
openapi-codegen/core/src/main/scala/sttp/tapir/codegen/EndpointGenerator.scala
case many =>
if (many.map(_.code).distinct.size != many.size) bail("Cannot construct schema for multiple responses with same status code")
val oneOfs = many.map { m =>
val code = if (m.code == "default") "400" else m.code
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could've been better I think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean 200?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean, default should encompass other status codes; there's an implicit contract binding we take in our choice of code, but really this is explicitly an 'open' sort of specification.... I don't know how to express it well I'm afraid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamw adamw adamw left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hughsimpson @adamw